-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRC Generation Module #54
base: main
Are you sure you want to change the base?
Conversation
val crc1 = RegInit(Bits(8.W), 0.U) | ||
|
||
// Output signal registers | ||
val message_ready = RegInit(Bool(), true.B) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: camelcase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was told to use snake case by @rahulk29, but I can switch it to camelcase if that is recommended.
* and crc valid state low. CRC bits will be cleared to 0. | ||
* @group Signals | ||
*/ | ||
val reset = Input(Bool()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use implicit reset instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would I just be able to omit the reset declaration in the IO. I also changed it so that when a new message is latched, the crc output is cleared and set invalid, let me know if this is correct though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of architecture, we will probably need something that can run faster than one byte per cycle in order to keep up with the link's speed, and also it might be good to eventually have some sort of streaming capability so that we can feed the module bytes as soon as they are available, instead of collecting the entire word and summing it at once. This is fine for now though and we should probably wait until we better understand how the module will get used to determine what architecture changes to make.
crc1 := crc0 ^ lookup | ||
.table(crc1 ^ message_bits(width - 1, width - 8))(15, 8) | ||
crc0 := lookup.table(crc1 ^ message_bits(width - 1, width - 8))(7, 0) | ||
message_bits := message_bits << 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this endianness correct? This implementation treats message
as one big-endian integer and CRC's bytes from most-significant-byte to LSB (as documented); not sure if this is what we want though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a little unconventional compared to what I normally see for CRC calculations, but the results of this CRC generator match the results of the design the UCIe1.0 spec has in the appendix (where they just selectively xor the 1024 bits of the message for each of the 16 crc bits). I ran a simulation of the xor'ing method in python to generate the test cases I listed in the test module.
Since hardware types can only be instantiated inside of a module, the `table` member is made into a parentheses-less method so that the `VecInit` is constructed at the use site.
No description provided.